Added `as_super` methods to `PyRef` and `PyRefMut`. #4219

JRRudy1 · 2024-05-29T01:13:39Z

Introduction

This PR simplifies the process of accessing superclasses by adding an as_super method to PyRef and PyRefMut that lets you convert &PyRef<T> to &PyRef<T::BaseType and &mut PyRefMut<T> to &mut PyRefMut<T::BaseType>. This acts as a hybrid between the existing as_ref and into_super methods, providing both the by-reference ergonomics of as_ref with the super-superclass access of into_super while unifying the concepts under a more intuitive/idiomatic pattern.

What's wrong with `<PyRef<T> as AsRef<T::BaseType>>::as_ref`?

As mentioned in the docs, calling as_ref can give you &T::BaseType but not the super-superclass and beyond. But on top of that, AsRef::as_ref is a general-purpose method used throughout the Rust language, and it is not intuitive that this is how you do the pyo3-specific task of referencing the base class. Sure, the existence of an AsRef<T::BaseType> impl is useful if you want a define a function that is generic over any type that can be used to get &T::BaseType, but this should not be the primary method for doing so (just like Into::into should not be the primary method for converting &str to String ).

But what about `PyRef::into_super`?

Yes, into_super can also be used to get the super-superclass and beyond, but it consumes the PyRef<T> by value which can be awkward in many cases. If you have obj: PyRef<T> and just want to call a quick method on the super-superclass, your only option is to do obj.into_super().into_super().some_method(), which consumes your PyRef<T>. Now if you want to keep using the child class, hopefully you still have the Bound<T> around to borrow another PyRef (and incur the run-time borrow-checking overhead of doing so). This PR would instead let you instead do obj.as_super().as_super().some_method() and then keep using the original PyRef<T>.

That said, there is certainly still a place for the into_super method; calling as_super requires someone to retain ownership of the PyRef<T>, so you could not return the &PyRef<T::BaseType> from a function; for this you would need to use into_super and return PyRef<T::BaseType> by value. But under this system, the as_super and into_super methods have distinct purposes and idiomatic names that most Rust users will immediately recognize as doing the same thing but by-reference vs. by-value.

Implementation

`PyRef::as_super` and `PyRefMut::as_super` methods

The implementations of the new methods added in this PR (PyRef::as_super, PyRefMut::as_super, and the private PyRefMut::downgrade helper function) could literally be replaced by calls to std::mem::transmute (or the equivalent pointer casts); however, I broke them up a bit to make the safety logic more clear. For example, the PyRef::as_super method calls downcast_unchecked:: on the inner Bound<T> instead of just going straight from PyRef<T> to PyRef. The same was not done for PyRefMut::as_super, however, since it would required transmuting a shared reference &Bound to a mutable reference &mut PyRefMut which might be fine but feels sketchy.

`AsRef` and `AndMut` impls

A nice side effect of adding the purpose-built as_super methods was the ability to simplify the general-purpose AsRef and AsMut impls by simply re-borrowing the reference returned by the as_super call. This avoid the need for additional unsafe code in these impls. However, doing this for PyRefMut's AsRef impl required "downgrading" the PyRefMut into PyRef, which I encapsulated in a private PyRefMut::downgrade associated function.

Safety

The safety logic behind this PR is fairly simple, and can be summarized as the following:

I added #[repr(transparent)] to the PyRef/PyRefMut structs such that they are guaranteed to have the same layout as the Bound<T> they wrap. By extension, PyRef<T> and PyRefMut<T> also have the same layout as each other.
Bound<T> has the same layout as Bound<T::BaseType> and can be transmuted to it freely (this is essentially how downcast_unchecked works, after all).
The new methods only involve references to PyRef/PyRefMut, so the differences in Drop impls are not relevant; transmuting PyRefMut<T> to PyRef<T> would be problematic since dropping it would call release_borrow instead of release_borrow_mut, but dropping references does not call drop so transmuting &PyRefMut<T> to &PyRef<T> is fine.

As a result, the following pointer cast chains should be sound:

&PyRef<T> -> &Bound<T> -> &Bound -> &PyRef (where T: PyClass<BaseType=U>, U: PyClass)
&PyRefMut<T> -> &PyRef<T> -> &Bound<T> -> &Bound -> &PyRef (where T: PyClass<Frozen=False, BaseType=U>, U: PyClass)
&mut PyRefMut<T> -> &mut Bound<T> -> &mut Bound -> &mut PyRefMut (where T: PyClass<Frozen=False, BaseType=U>, U: PyClass<Frozen=False>)

…ocstrings and tests. The implementation of these methods also required adding `#[repr(transparent)]` to the `PyRef` and `PyRefMut` structs.

… use the new `as_super` methods. Added the `PyRefMut::downgrade` associated function for converting `&PyRefMut` to `&PyRef`. Updated tests and docstrings to better demonstrate the new functionality.

davidhewitt

Thanks, this makes a lot of sense! Just a brief thought on the implementation details and one typo...

src/pycell.rs

davidhewitt

WIth the new ptr_from_ref, let's please clean up the implementation to the newer style and then 👍 let's merge!

src/pycell.rs

…rade` to use `.cast()` instead of `as _` pointer casts. Fixed typo.

…ref` for the initial cast to `*const _` instead of `as _` casts.

JRRudy1 · 2024-06-08T15:55:55Z

Thanks for the review David! I made the changes as requested. I definitely like your approach of ptr_from_ref and .cast over as casts. I just have a couple question about the new implementation:

How do you feel about the way I split up the casts and used explicit types, such as in the PyRef::as_super method? I personally prefer being explicit in these cases to help justify the safety, but I could also see the value in being concise by using a single .cast with implicit types.
Do you see any value in making PyRefMut::downgrade public? I suppose it could be useful in some niche cases.

A couple other thoughts:

IMO the User Guide should be updated to suggest as_super (alongside into_super) instead of as_ref for getting the base class. If you agree, I can make the change in another PR (or this one, whichever you prefer).
I like your ptr_from_ref approach with the ultimate goal of using ptr::from_ref when the MSRV allows. But I wonder if it would be worthwhile to add an analogous ptr_from_mut function for the &mut T -> *mut T conversion as well, even if there won't be a stdlib ptr::from_mut function for a while.

JRRudy1 · 2024-06-08T16:13:12Z

even if there won't be a stdlib ptr::from_mut function for a while.

Actually it seems like ptr::from_ref and ptr::from_mut are being stabilized together, so I definitely think it makes sense to add a ptr_from_mut function alongside ptr_from_ref unless I'm missing something.

davidhewitt · 2024-06-08T16:19:49Z

Great questions!

* How do you feel about the way I split up the casts and used explicit types, such as in the `PyRef::as_super` method? I personally prefer being explicit in these cases to help justify the safety, but I could also see the value in being concise by using a single `.cast` with implicit types.

I went back and forth over this, I see upsides to both. Given the runtime cost should be the same I'm happy to keep what you already have here.

* Do you see any value in making `PyRefMut::downgrade` public? I suppose it could be useful in some niche cases.

Sure thing, we can expose it. Maybe as a follow up PR?

* IMO the User Guide should be updated to suggest `as_super` (alongside `into_super`) instead of `as_ref` for getting the base class. If you agree, I can make the change in another PR (or this one, whichever you prefer).

It would be great to update the documentation in this PR, yes please.

* I like your `ptr_from_ref` approach with the ultimate goal of using `ptr::from_ref` when the MSRV allows. But I wonder if it would be worthwhile to add an analogous `ptr_from_mut` function for the `&mut T` -> `*mut T` conversion as well, even if there won't be a stdlib `ptr::from_mut` function for a while.

I'd skipped ptr_from_mut purely because I wanted to see what fellow maintainers thought with just one part of the work initially. I'd support adding it in this PR and using it in the PyRefMut implementation 👍

…tr_from_ref` added in PR PyO3#4240. Updated `PyRefMut::as_super` to use this method instead of `as *mut _`.

… class instead of `as_ref`, and updated the subsequent example/doctest to demonstrate this functionality.

JRRudy1 · 2024-06-08T20:57:07Z

I'll work on fixing the checks, but in the meantime:

It would be great to update the documentation in this PR, yes please.

I updated the docs, as well as the example code that follows it. Let me know if you have any concerns about the wording.

I'd support adding it in this PR and using it in the PyRefMut implementation 👍

I added this as well. A couple notes however:

ptr_from_mut can't be const like ptr_from_ref due to the &mut. I don't think this has a runtime cost, and only limits where it can be used, but I could be wrong.
On that note... the upcoming ptr::from_ref doesn't actually appear to be const either. If you want to replace ptr_from_ref with ptr::from_ref when MSRV allows, then maybe ptr_from_ref should also not be const. Otherwise code could be written that relies on its const-ness, which would break when making the change.

davidhewitt

Looks perfect, thanks!

I'll follow up sometime to remove constness from ptr_from_ref; thanks for flagging that. I don't think lack of constness will change the runtime cost.

davidhewitt · 2024-06-09T07:15:59Z

newsfragments/4219.added.md

@@ -0,0 +1,3 @@
+- Added `as_super` methods to `PyRef` and `PyRefMut` for accesing the base class by reference
+- Updated user guide to recommend `as_super` for referencing the base class instead of `as_ref`


No need to include docs changes or internal changes here, but as this PR is otherwise great let's just leave these points here and I'll tidy up during release.

Good to know, I'll keep that in mind next time. Thanks for the merge!

davidhewitt · 2024-06-11T07:59:06Z

On that note... the upcoming ptr::from_ref doesn't actually appear to be const either. If you want to replace ptr_from_ref with ptr::from_ref when MSRV allows, then maybe ptr_from_ref should also not be const. Otherwise code could be written that relies on its const-ness, which would break when making the change.

Just following up here, it looks like ptr::from_ref and ptr::from_mut are const - https://doc.rust-lang.org/stable/std/ptr/fn.from_mut.html

So no changes needed in PyO3! 😄

JRRudy1 · 2024-06-11T13:12:01Z

Just following up here, it looks like ptr::from_ref and ptr::from_mut are const - https://doc.rust-lang.org/stable/std/ptr/fn.from_mut.html

So no changes needed in PyO3! 😄

Well I guess I'm losing it.... I swear I saw it not being const 😅 maybe I was looking at an earlier draft of it or something.

But that's good to hear, I was very surprised when I thought it wasn't.

JRRudy1 added 6 commits May 28, 2024 15:57

Added PyRef::as_super and PyRefMut::as_super methods, including d…

477fa6f

…ocstrings and tests. The implementation of these methods also required adding `#[repr(transparent)]` to the `PyRef` and `PyRefMut` structs.

Added newsfragment entry.

6d27b2d

Changed the AsRef/AsMut impls for PyRef and PyRefMut to…

42d7ca9

… use the new `as_super` methods. Added the `PyRefMut::downgrade` associated function for converting `&PyRefMut` to `&PyRef`. Updated tests and docstrings to better demonstrate the new functionality.

Fixed newsfragment filename.

189d92e

Removed unnecessary re-borrows flagged by clippy.

6596cd6

retrigger checks

8c44d28

JRRudy1 mentioned this pull request May 29, 2024

Added PyRefMap and PyRefMapMut for borrowing data nested within a PyRef/PyRefMut #4203

Draft

davidhewitt reviewed Jun 7, 2024

View reviewed changes

src/pycell.rs Outdated Show resolved Hide resolved

src/pycell.rs Outdated Show resolved Hide resolved

davidhewitt reviewed Jun 8, 2024

View reviewed changes

src/pycell.rs Outdated Show resolved Hide resolved

JRRudy1 and others added 5 commits June 8, 2024 09:46

Updated PyRef::as_super, PyRefMut::as_super, and `PyRefMut::downg…

23c7836

…rade` to use `.cast()` instead of `as _` pointer casts. Fixed typo.

Merge branch 'PyO3:main' into pyref_as_super

58a6ee7

Merge remote-tracking branch 'origin/main' into pyref_as_super

e97b28a

Updated PyRef::as_super and PyRefMut::downgrade to use `ptr_from_…

4e345f3

…ref` for the initial cast to `*const _` instead of `as _` casts.

Merge remote-tracking branch 'origin/pyref_as_super' into pyref_as_super

6bef736

JRRudy1 added 4 commits June 8, 2024 11:43

Added pyo3::internal_tricks::ptr_from_mut function alongside the `p…

9c32d77

…tr_from_ref` added in PR PyO3#4240. Updated `PyRefMut::as_super` to use this method instead of `as *mut _`.

Updated the user guide to recommend as_super for accessing the base…

1d3e41a

… class instead of `as_ref`, and updated the subsequent example/doctest to demonstrate this functionality.

Improved tests for the as_super methods.

bf4e872

Updated newsfragment to include additional changes.

0fc9143

Fixed formatting.

f85c83a

davidhewitt approved these changes Jun 9, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Jun 9, 2024

Merged via the queue into PyO3:main with commit d2dca21 Jun 9, 2024
41 checks passed

JRRudy1 mentioned this pull request Jul 14, 2024

Added as_super and into_super methods for Bound<T: PyClass> #4351

Merged

JRRudy1 deleted the pyref_as_super branch July 16, 2024 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `as_super` methods to `PyRef` and `PyRefMut`. #4219

Added `as_super` methods to `PyRef` and `PyRefMut`. #4219

JRRudy1 commented May 29, 2024 •

edited

Loading

davidhewitt left a comment

davidhewitt left a comment

JRRudy1 commented Jun 8, 2024

JRRudy1 commented Jun 8, 2024

davidhewitt commented Jun 8, 2024

JRRudy1 commented Jun 8, 2024 •

edited

Loading

davidhewitt left a comment •

edited

Loading

davidhewitt Jun 9, 2024

JRRudy1 Jun 11, 2024

davidhewitt commented Jun 11, 2024

JRRudy1 commented Jun 11, 2024

		@@ -0,0 +1,3 @@
		- Added `as_super` methods to `PyRef` and `PyRefMut` for accesing the base class by reference
		- Updated user guide to recommend `as_super` for referencing the base class instead of `as_ref`

Added as_super methods to PyRef and PyRefMut. #4219

Added as_super methods to PyRef and PyRefMut. #4219

Conversation

JRRudy1 commented May 29, 2024 • edited Loading

Introduction

What's wrong with <PyRef<T> as AsRef<T::BaseType>>::as_ref?

But what about PyRef::into_super?

Implementation

PyRef::as_super and PyRefMut::as_super methods

AsRef and AndMut impls

Safety

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

JRRudy1 commented Jun 8, 2024

JRRudy1 commented Jun 8, 2024

davidhewitt commented Jun 8, 2024

JRRudy1 commented Jun 8, 2024 • edited Loading

davidhewitt left a comment • edited Loading

Choose a reason for hiding this comment

davidhewitt Jun 9, 2024

Choose a reason for hiding this comment

JRRudy1 Jun 11, 2024

Choose a reason for hiding this comment

davidhewitt commented Jun 11, 2024

JRRudy1 commented Jun 11, 2024

Added `as_super` methods to `PyRef` and `PyRefMut`. #4219

Added `as_super` methods to `PyRef` and `PyRefMut`. #4219

JRRudy1 commented May 29, 2024 •

edited

Loading

What's wrong with `<PyRef<T> as AsRef<T::BaseType>>::as_ref`?

But what about `PyRef::into_super`?

`PyRef::as_super` and `PyRefMut::as_super` methods

`AsRef` and `AndMut` impls

JRRudy1 commented Jun 8, 2024 •

edited

Loading

davidhewitt left a comment •

edited

Loading